UI Cleanup: Visible Keys and Objective Removal - #25
Conversation
…moval - Removed `type="password"` from API key inputs to show stored values. - Removed the 'Objective' header from the main chat interface. - Verified all debug logs and redundant UI messages are removed. - Finalized sidebar restructuring with tabbed Chats/Config.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe PR systematically suppresses error output across the application while reorganizing the Streamlit UI into a sidebar with container-based chat and configuration panels. Session state no longer tracks minimization, intermediate automation messages are removed, and two unused utility functions are deleted. Four new comprehensive test suites validate all implementation changes. ChangesError Suppression & Sidebar UI Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoClean up UI and remove debug logging across application
WalkthroughsDescription• Remove debug logs and print statements throughout codebase • Make API keys visible by removing password input masking • Remove redundant 'Objective' header from chat interface • Restructure sidebar into tabbed interface for Chats and Config • Remove minimize/expand configuration panel functionality • Clean up exception handling by removing verbose error logging Diagramflowchart LR
A["Debug Logs & Print Statements"] -->|Remove| B["Clean Codebase"]
C["Password-Masked API Keys"] -->|Make Visible| D["Visible Configuration"]
E["Objective Header"] -->|Remove| F["Simplified Chat UI"]
G["Sidebar Layout"] -->|Restructure to Tabs| H["Tabbed Control Center"]
B --> I["Final UI Cleanup"]
D --> I
F --> I
H --> I
File Changes1. app.py
|
Code Review by Qodo
1. API keys unmasked
|
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| mistral_api_key = st.text_input( | ||
| "Mistral API Key", | ||
| value=st.session_state.mistral_api_key, | ||
| type="password", | ||
| help="Enter your Mistral AI API key", | ||
| key="mistral_input" | ||
| ) |
There was a problem hiding this comment.
1. Api keys unmasked 🐞 Bug ⛨ Security
The configuration panel now renders stored Mistral/Firecrawl API keys as plain text by removing password masking, making accidental disclosure likely (screen share, screenshots, shoulder-surfing). Because the values are also loaded from and persisted to cookies, the raw keys will be displayed whenever present.
Agent Prompt
## Issue description
API keys are rendered in clear text because `st.text_input(..., type="password")` was removed.
## Issue Context
Keys are loaded from cookies into session state and then passed as `value=` to the inputs, so any existing key immediately appears on-screen.
## Fix Focus Areas
- app.py[175-259]
### Implementation sketch
- Re-introduce `type="password"` for both key inputs.
- If visibility is desired, add a `st.checkbox("Show API keys")` and conditionally set `type=None` only when explicitly enabled (default to masked).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4da1527bd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Detect and highlight elements | ||
| annotated_image_path = st.session_state.element_detector.detect_and_annotate_elements(screenshot_path, st.session_state.browser) | ||
| add_message("assistant", annotated_image_path, "image", "Elements detected and indexed") | ||
| add_message("assistant", annotated_image_path, "image") |
There was a problem hiding this comment.
take_screenshot_and_analyze now stores only the annotated image message, but detect_and_annotate_elements writes that file as a copy and leaves the original screenshot on disk. Because delete_chat_screenshots deletes only image paths present in chat messages, every successful automation step leaves an untracked raw screenshot in screenshots/, causing disk growth and stale sensitive captures over time; either persist screenshot_path for cleanup or delete it immediately after annotation succeeds.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app.py (1)
208-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAPI keys are now exposed in plain text without password masking.
While the PR explicitly states this is intentional, removing password masking from API key inputs creates significant security risks:
- Shoulder surfing: Keys are visible to anyone nearby
- Screen sharing: Keys exposed during support sessions or demos
- Screenshots/recordings: Accidental capture of credentials
- Live streaming: Easy to forget keys are visible
Industry best practice is to mask credentials by default. If users need to verify their input, add a reveal toggle (e.g., an eye icon) rather than showing keys in plain text by default.
🔒 Recommended fix: restore password masking
mistral_api_key = st.text_input( "Mistral API Key", value=st.session_state.mistral_api_key, + type="password", help="Enter your Mistral AI API key", key="mistral_input" )Apply the same fix to the Firecrawl API key input at line 241:
firecrawl_api_key = st.text_input( "Firecrawl API Key", value=st.session_state.firecrawl_api_key, + type="password", help="Enter your Firecrawl API key", key="firecrawl_input" )Also applies to: 241-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app.py` around lines 208 - 213, Restore password masking for API key inputs by changing the Streamlit text_input for the Mistral key (key="mistral_input" / variable mistral_api_key) to use the password input type and add a reveal toggle UI so users can optionally unmask; apply the same change to the Firecrawl API key input (key likely "firecrawl_input" / variable firecrawl_api_key). Ensure the inputs keep their session_state defaults (st.session_state.mistral_api_key and st.session_state.firecrawl_api_key) and implement the reveal toggle (eye icon or checkbox) to switch between masked and plain text modes without leaving keys visible by default.
🧹 Nitpick comments (1)
utils.py (1)
38-39: ⚡ Quick winAmbiguous return value:
Noneconflates "file not found" with "parse error".Returning
Nonefor all exceptions makes it impossible for callers to distinguish between expected conditions (file doesn't exist yet) and errors that need attention (corrupted JSON, permission denied). Consider returning a tuple(data, error)or raising exceptions and letting callers handle expected cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils.py` around lines 38 - 39, The catch-all "except Exception: return None" in the function that reads/parses JSON (the except Exception block) hides distinct outcomes; change the function to return a tuple (data, error) instead of a bare None: on success return (data, None), catch FileNotFoundError and return (None, None) to indicate "no file yet", and for JSONDecodeError/PermissionError return (None, err) (or re-raise if you prefer strict handling); update callers of this reader to handle the (data, error) tuple appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.py`:
- Around line 98-99: The current broad "except Exception: pass" in
save_chats_to_local and load_chats_from_local silently swallows storage errors
and can cause data loss; change these handlers to catch specific storage-related
exceptions (e.g., DOMException/StorageError or more specific exceptions raised
by your environment) instead of a bare Exception, log the error details to your
logger, and surface a UI-facing warning/state (e.g., set a persistenceError flag
or call showPersistenceWarning()) so the app can display a warning to the user;
ensure the functions return a success boolean or propagate the error state so
callers can react appropriately.
In `@element_detector.py`:
- Around line 78-79: The current bare except that returns screenshot_path
swallows errors; change the except block that catches Exception (the one
returning screenshot_path) to "except Exception as e:" and log the full
error/stack trace via the module logger (e.g., logger.exception or
logging.exception) so diagnostics are preserved, then return a tuple
(screenshot_path, False) instead of the raw path so callers can distinguish
failure vs success; update any callers of the function to handle the (path,
success_flag) return or alternatively keep a backward-compatible code path that
returns just the path while also emitting the logged exception.
---
Outside diff comments:
In `@app.py`:
- Around line 208-213: Restore password masking for API key inputs by changing
the Streamlit text_input for the Mistral key (key="mistral_input" / variable
mistral_api_key) to use the password input type and add a reveal toggle UI so
users can optionally unmask; apply the same change to the Firecrawl API key
input (key likely "firecrawl_input" / variable firecrawl_api_key). Ensure the
inputs keep their session_state defaults (st.session_state.mistral_api_key and
st.session_state.firecrawl_api_key) and implement the reveal toggle (eye icon or
checkbox) to switch between masked and plain text modes without leaving keys
visible by default.
---
Nitpick comments:
In `@utils.py`:
- Around line 38-39: The catch-all "except Exception: return None" in the
function that reads/parses JSON (the except Exception block) hides distinct
outcomes; change the function to return a tuple (data, error) instead of a bare
None: on success return (data, None), catch FileNotFoundError and return (None,
None) to indicate "no file yet", and for JSONDecodeError/PermissionError return
(None, err) (or re-raise if you prefer strict handling); update callers of this
reader to handle the (data, error) tuple appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 122be4e7-7bef-4d80-bc71-4ec62dcbcece
📒 Files selected for processing (4)
app.pybrowser_automation.pyelement_detector.pyutils.py
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silent localStorage failures could result in data loss.
Both save_chats_to_local() and load_chats_from_local() now suppress all exceptions. Users will not know if their chat history failed to persist due to quota limits, browser privacy settings, or other localStorage errors. Consider at minimum showing a warning indicator in the UI when persistence fails, even if detailed errors aren't logged.
Also applies to: 120-121
🧰 Tools
🪛 Ruff (0.15.12)
[error] 98-99: try-except-pass detected, consider logging the exception
(S110)
[warning] 98-98: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.py` around lines 98 - 99, The current broad "except Exception: pass" in
save_chats_to_local and load_chats_from_local silently swallows storage errors
and can cause data loss; change these handlers to catch specific storage-related
exceptions (e.g., DOMException/StorageError or more specific exceptions raised
by your environment) instead of a bare Exception, log the error details to your
logger, and surface a UI-facing warning/state (e.g., set a persistenceError flag
or call showPersistenceWarning()) so the app can display a warning to the user;
ensure the functions return a success boolean or propagate the error state so
callers can react appropriately.
| except Exception: | ||
| return screenshot_path # Return original if annotation fails |
There was a problem hiding this comment.
Silent failure eliminates all diagnostic information.
The exception handler now suppresses all errors without providing any user feedback. Users cannot distinguish between successful annotation with no elements versus annotation failures due to file corruption, PIL errors, or font loading issues. In a browser automation context, this observability gap makes debugging significantly harder.
Consider at minimum returning a tuple (path, success_flag) or logging to a debug channel that developers can enable.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 78-78: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@element_detector.py` around lines 78 - 79, The current bare except that
returns screenshot_path swallows errors; change the except block that catches
Exception (the one returning screenshot_path) to "except Exception as e:" and
log the full error/stack trace via the module logger (e.g., logger.exception or
logging.exception) so diagnostics are preserved, then return a tuple
(screenshot_path, False) instead of the raw path so callers can distinguish
failure vs success; update any callers of the function to handle the (path,
success_flag) return or alternatively keep a backward-compatible code path that
returns just the path while also emitting the logged exception.
|
✅ Unit tests committed locally. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test_app_changes.py`:
- Around line 25-42: The test mutates global import state by inserting MagicMock
objects into sys.modules using _STUB_MODULES/_mod_name without restoring
originals; change this to save each original sys.modules.get(_mod_name) before
stubbing, replace or insert the MagicMock for the duration of the test (or wrap
in a fixture/context manager), and then restore the saved original (deleting the
key if it didn't exist before) after the test completes so sys.modules is
returned to its prior state; look for the _STUB_MODULES list, the loop over
_mod_name, sys.modules, and MagicMock to implement the save/restore logic or
move it into setup/teardown.
In `@test_browser_automation.py`:
- Around line 242-245: The test currently swallows all exceptions around the
call to _run_screenshot_with_bytes(self.ba, unknown_bytes) (and the similar
block later), which masks real failures; remove the broad try/except or replace
it with an explicit assertion of the expected error using pytest.raises (or
assertRaises) for the specific exception type you expect from
_run_screenshot_with_bytes, so unexpected exceptions will fail the test and the
intended error behavior is asserted instead.
- Around line 22-25: The top-level mutation of sys.modules via _STUBS and
MagicMock leaks import-state across tests; change this to a temporary,-restored
approach: replace the direct loop with a scoped mechanism (e.g., use
unittest.mock.patch.dict on sys.modules or a pytest fixture that records
original = {k: sys.modules.get(k) for k in _STUBS}, inserts MagicMock() for
missing keys, yields to run the test, then restores originals or deletes
inserted keys in a finally block). Target the symbols _STUBS, sys.modules, and
MagicMock when implementing the patch/dict or fixture so the mocked modules are
removed/restored after each test run.
In `@test_element_detector.py`:
- Around line 140-150: The test
test_returns_original_when_browser_raises_exception uses a corrupt temp image so
the method detect_and_annotate_elements may return the original path for
image-parsing reasons rather than because mock_browser.get_interactable_elements
raised; replace the invalid image with a small valid PNG (so image loading
succeeds), keep mock_browser.get_interactable_elements.side_effect =
RuntimeError("fail"), call detect_and_annotate_elements and assert the return
equals the original path, and additionally assert
mock_browser.get_interactable_elements was called (or session_id accessed) to
ensure the browser-exception branch was exercised.
- Around line 20-34: The test currently injects global stubs into sys.modules
via _STUBS and PIL mocks without restoring originals, which can leak into other
tests; update the setup to save original entries for each module in _STUBS and
for "PIL"/"PIL.Image"/"PIL.ImageDraw"/"PIL.ImageFont" (e.g., store a dict of
originals keyed by module name) before assigning MagicMock, and add teardown
logic (or use pytest's fixture/monkeypatch) to restore those originals (or
delete the injected mocks) and reset _PIL_AVAILABLE/_real_pil_image
appropriately after the test completes so module state is deterministic for
subsequent tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1f2b8b2-e80b-45f1-a926-b61f891fabfc
📒 Files selected for processing (4)
test_app_changes.pytest_browser_automation.pytest_element_detector.pytest_utils.py
| _STUB_MODULES = [ | ||
| "streamlit", | ||
| "extra_streamlit_components", | ||
| "streamlit_local_storage", | ||
| "browser_automation", | ||
| "mistral_client", | ||
| "fireworks_client", | ||
| "element_detector", | ||
| "cv2", | ||
| "PIL", | ||
| "PIL.Image", | ||
| "firecrawl", | ||
| "requests", | ||
| ] | ||
| for _mod_name in _STUB_MODULES: | ||
| if _mod_name not in sys.modules: | ||
| sys.modules[_mod_name] = MagicMock() | ||
|
|
There was a problem hiding this comment.
Restore sys.modules after stubbing to avoid cross-test contamination.
Line 25 mutates global import state and never restores it. This can make unrelated tests pass/fail depending on execution order.
Suggested fix
+_ORIGINAL_MODULES = {name: sys.modules.get(name) for name in _STUB_MODULES}
for _mod_name in _STUB_MODULES:
if _mod_name not in sys.modules:
sys.modules[_mod_name] = MagicMock()
+
+def tearDownModule():
+ for name, original in _ORIGINAL_MODULES.items():
+ if original is None:
+ sys.modules.pop(name, None)
+ else:
+ sys.modules[name] = original📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _STUB_MODULES = [ | |
| "streamlit", | |
| "extra_streamlit_components", | |
| "streamlit_local_storage", | |
| "browser_automation", | |
| "mistral_client", | |
| "fireworks_client", | |
| "element_detector", | |
| "cv2", | |
| "PIL", | |
| "PIL.Image", | |
| "firecrawl", | |
| "requests", | |
| ] | |
| for _mod_name in _STUB_MODULES: | |
| if _mod_name not in sys.modules: | |
| sys.modules[_mod_name] = MagicMock() | |
| _STUB_MODULES = [ | |
| "streamlit", | |
| "extra_streamlit_components", | |
| "streamlit_local_storage", | |
| "browser_automation", | |
| "mistral_client", | |
| "fireworks_client", | |
| "element_detector", | |
| "cv2", | |
| "PIL", | |
| "PIL.Image", | |
| "firecrawl", | |
| "requests", | |
| ] | |
| _ORIGINAL_MODULES = {name: sys.modules.get(name) for name in _STUB_MODULES} | |
| for _mod_name in _STUB_MODULES: | |
| if _mod_name not in sys.modules: | |
| sys.modules[_mod_name] = MagicMock() | |
| def tearDownModule(): | |
| for name, original in _ORIGINAL_MODULES.items(): | |
| if original is None: | |
| sys.modules.pop(name, None) | |
| else: | |
| sys.modules[name] = original |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_app_changes.py` around lines 25 - 42, The test mutates global import
state by inserting MagicMock objects into sys.modules using
_STUB_MODULES/_mod_name without restoring originals; change this to save each
original sys.modules.get(_mod_name) before stubbing, replace or insert the
MagicMock for the duration of the test (or wrap in a fixture/context manager),
and then restore the saved original (deleting the key if it didn't exist before)
after the test completes so sys.modules is returned to its prior state; look for
the _STUB_MODULES list, the loop over _mod_name, sys.modules, and MagicMock to
implement the save/restore logic or move it into setup/teardown.
| _STUBS = ["firecrawl", "PIL", "PIL.Image", "cv2"] | ||
| for _mod in _STUBS: | ||
| if _mod not in sys.modules: | ||
| sys.modules[_mod] = MagicMock() |
There was a problem hiding this comment.
Prevent global import-state leakage from test stubs.
These top-level sys.modules mutations are not restored, which can make downstream tests order-dependent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_browser_automation.py` around lines 22 - 25, The top-level mutation of
sys.modules via _STUBS and MagicMock leaks import-state across tests; change
this to a temporary,-restored approach: replace the direct loop with a scoped
mechanism (e.g., use unittest.mock.patch.dict on sys.modules or a pytest fixture
that records original = {k: sys.modules.get(k) for k in _STUBS}, inserts
MagicMock() for missing keys, yields to run the test, then restores originals or
deletes inserted keys in a finally block). Target the symbols _STUBS,
sys.modules, and MagicMock when implementing the patch/dict or fixture so the
mocked modules are removed/restored after each test run.
| try: | ||
| _run_screenshot_with_bytes(self.ba, unknown_bytes) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Avoid try/except Exception: pass in assertions.
These blocks can hide real failures and let the test pass vacuously. Let unexpected exceptions fail the test (or assert a specific expected exception).
Also applies to: 282-285
🧰 Tools
🪛 Ruff (0.15.12)
[error] 244-245: try-except-pass detected, consider logging the exception
(S110)
[warning] 244-244: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_browser_automation.py` around lines 242 - 245, The test currently
swallows all exceptions around the call to _run_screenshot_with_bytes(self.ba,
unknown_bytes) (and the similar block later), which masks real failures; remove
the broad try/except or replace it with an explicit assertion of the expected
error using pytest.raises (or assertRaises) for the specific exception type you
expect from _run_screenshot_with_bytes, so unexpected exceptions will fail the
test and the intended error behavior is asserted instead.
| _STUBS = ["numpy", "cv2"] | ||
| for _mod in _STUBS: | ||
| if _mod not in sys.modules: | ||
| sys.modules[_mod] = MagicMock() | ||
|
|
||
| # We keep PIL real (it's in requirements.txt) but fall back to a mock if absent. | ||
| try: | ||
| from PIL import Image as _real_pil_image | ||
| _PIL_AVAILABLE = True | ||
| except ImportError: | ||
| sys.modules["PIL"] = MagicMock() | ||
| sys.modules["PIL.Image"] = MagicMock() | ||
| sys.modules["PIL.ImageDraw"] = MagicMock() | ||
| sys.modules["PIL.ImageFont"] = MagicMock() | ||
| _PIL_AVAILABLE = False |
There was a problem hiding this comment.
Add module-stub cleanup to keep test isolation deterministic.
Line 20 writes global stubs into sys.modules but never restores originals. This can leak mocked dependencies into other test files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_element_detector.py` around lines 20 - 34, The test currently injects
global stubs into sys.modules via _STUBS and PIL mocks without restoring
originals, which can leak into other tests; update the setup to save original
entries for each module in _STUBS and for
"PIL"/"PIL.Image"/"PIL.ImageDraw"/"PIL.ImageFont" (e.g., store a dict of
originals keyed by module name) before assigning MagicMock, and add teardown
logic (or use pytest's fixture/monkeypatch) to restore those originals (or
delete the injected mocks) and reset _PIL_AVAILABLE/_real_pil_image
appropriately after the test completes so module state is deterministic for
subsequent tests.
| def test_returns_original_when_browser_raises_exception(self): | ||
| """If get_element_positions_from_browser raises, original path is returned.""" | ||
| with tempfile.NamedTemporaryFile(suffix=".png", delete=False) as f: | ||
| f.write(b"not a real image") | ||
| path = f.name | ||
| try: | ||
| mock_browser = MagicMock() | ||
| mock_browser.session_id = "sess" | ||
| mock_browser.get_interactable_elements.side_effect = RuntimeError("fail") | ||
| result = self.detector.detect_and_annotate_elements(path, mock_browser) | ||
| self.assertEqual(result, path) |
There was a problem hiding this comment.
This test can pass without actually exercising the browser-error path.
Because the screenshot is intentionally invalid, returning the original path can happen even if the browser exception branch is never reached. Use a valid image and assert the browser call happened.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test_element_detector.py` around lines 140 - 150, The test
test_returns_original_when_browser_raises_exception uses a corrupt temp image so
the method detect_and_annotate_elements may return the original path for
image-parsing reasons rather than because mock_browser.get_interactable_elements
raised; replace the invalid image with a small valid PNG (so image loading
succeeds), keep mock_browser.get_interactable_elements.side_effect =
RuntimeError("fail"), call detect_and_annotate_elements and assert the return
equals the original path, and additionally assert
mock_browser.get_interactable_elements was called (or session_id accessed) to
ensure the browser-exception branch was exercised.
1 participant
This update completes the UI cleanup by making stored API keys visible in the configuration panel, removing the redundant 'Objective' header, and ensuring all debug logs and intermediate step messages are gone. It also includes the previously implemented sidebar restructuring into a tabbed interface.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests